Skip to content

Assignments week 1 Ilias Khugaev #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Narpimers
Copy link

No description provided.

Copy link

@Pandelissym Pandelissym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work for the first assignment! Your API returns exactly what its supposed to and fits the requirements.

I have left a few suggestions and comments to improve your code!

import express from 'express';

const app = express();
const port = process.env.PORT || 3000;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice use of environmental variables here!



app.get('/', (req, res) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this whiteline here. Usually we start writing code on the first line like you have done in the post request. Its good to keep all our functions consistent to improve readability.


res.sendStatus = 200;
res.send('Hello from backend to frontend!');
res.end;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Express' res.end is a function so you would have to call it like this: res.end(). Calling the function is not necessary here. When you call res.send with your data it calls res.end under the hood. I would remove it as it is unnecessary and we want to keep the code as clean and readable as possible!


app.get('/', (req, res) => {

res.sendStatus = 200;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on thinking about the status code! However, res.sendStatus is a function so to call it and set the status code you would have to do this:

res.sendStatus(200)

What you are doing in your code is actually overwriting this function and setting the sendStatus field to 200, so if you try to call res.sendStatus() after this line you will see it fails as it is not a function. You can try this out yourself!

One thing to note is that res.sendStatus(code) immediately will send the response back to the client with the specified status code. So whatever you do after that line to the response object will not have an effect/throw an error. This is not the behavior we want here.

A good thing to point out is that res.send(data) which you call below, automatically sets the status code to 200 and returns the response to the client. So you do not need to manually set the status code. This is nice as it allows us to write less lines of code. If you wanted to overwrite that status code you could call res.statusCode = something to set it before returning the response.

res.send('Hello from backend to frontend!');
res.end;
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the 3 white lines, and keep only 1. Good work on separating the functions and leaving space to improve readability. By convention we usually leave one line between our functions to keep a balance between readability of our code and file size. You can imagine if we had more functions in this file and each had 3 white lines separating them it would become harder and harder to scroll down and move between functions!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good afternoon! Thank you for your work — you were absolutely right about res.sendStatus(200). I actually meant to write res.statusCode = 200;. Based on your comments, I decided not to add the status code explicitly, since res.send() already sets it automatically for me. Thank you again for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants